fix(auth): enforce ADMIN on github/token and MEMBER/ADMIN on write routes - #1525
Open
Satyam296 wants to merge 14 commits into
Open
fix(auth): enforce ADMIN on github/token and MEMBER/ADMIN on write routes#1525Satyam296 wants to merge 14 commits into
Satyam296 wants to merge 14 commits into
Conversation
…te routes (traceroot-ai#1504) GET /api/github/token session branch now requires ADMIN — previously any workspace VIEWER could obtain a live GitHub App installation token scoped to the customer's connected repos. This brings it in line with every other github/* mutation route (connect/disconnect/login/callback all gate on ADMIN). Same class fixed across the remaining affected routes: - detectors POST / PATCH → MEMBER (create / edit) - detectors [detectorId] DELETE → ADMIN - ai/sessions POST → MEMBER (create session) - ai/sessions/[sessionId] DELETE → ADMIN - ai/sessions/[sessionId]/messages POST → MEMBER (run agent, spends LLM budget) Read-only GET routes are intentionally unchanged (any workspace member may read). The github/token internal-secret branch is unaffected. Tests: added role-gate assertions (VIEWER → 403, allowed role → success) for each mutating handler, asserting requireProjectAccess / requireWorkspaceMembership are called with the correct minRole string. Closes traceroot-ai#1504
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
No issues found across 12 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Client
participant API as API Route Handlers
participant Auth as auth-helpers
participant GitHub as GitHub Token API
participant Proj as Project API
participant Detectors as Detectors API
participant AI as AI Sessions API
Note over Client,AI: NEW: Role-gated routes
alt GET /api/github/token (session path)
Client->>API: GET /api/github/token?workspaceId=xxx
API->>Auth: requireAuth()
Auth-->>API: { user }
API->>Auth: requireWorkspaceMembership(user.id, workspaceId, "ADMIN")
alt ADMIN role
Auth-->>API: { member }
API->>GitHub: fetch installation token
GitHub-->>API: token
API-->>Client: 200 + token
else VIEWER or MEMBER
Auth-->>API: { error: 403 }
API-->>Client: 403 Forbidden
end
end
alt POST /api/projects/[projectId]/detectors
Client->>API: POST /detectors (create)
API->>Auth: requireAuth()
API->>Auth: requireProjectAccess(user.id, projectId, "MEMBER")
alt MEMBER or ADMIN
Auth-->>API: { project }
API->>Detectors: create detector
Detectors-->>API: detector
API-->>Client: 201 Created
else VIEWER
Auth-->>API: { error: 403 }
API-->>Client: 403 Forbidden
end
end
alt PATCH /api/projects/[projectId]/detectors/[detectorId]
Client->>API: PATCH /detectors/[id] (update)
API->>Auth: requireAuth()
API->>Auth: requireProjectAccess(user.id, projectId, "MEMBER")
alt MEMBER or ADMIN
Auth-->>API: { project }
API->>Detectors: update detector
Detectors-->>API: updated
API-->>Client: 200 OK
else VIEWER
Auth-->>API: { error: 403 }
API-->>Client: 403 Forbidden
end
end
alt DELETE /api/projects/[projectId]/detectors/[detectorId]
Client->>API: DELETE /detectors/[id]
API->>Auth: requireAuth()
API->>Auth: requireProjectAccess(user.id, projectId, "ADMIN")
alt ADMIN
Auth-->>API: { project }
API->>Detectors: delete detector
Detectors-->>API: deleted
API-->>Client: 200 OK
else MEMBER or VIEWER
Auth-->>API: { error: 403 }
API-->>Client: 403 Forbidden
end
end
alt POST /api/projects/[projectId]/ai/sessions
Client->>API: POST /ai/sessions (create)
API->>Auth: requireAuth()
API->>Auth: requireProjectAccess(user.id, projectId, "MEMBER")
alt MEMBER or ADMIN
Auth-->>API: { project }
API->>AI: create session
AI-->>API: session
API-->>Client: 201 Created
else VIEWER
Auth-->>API: { error: 403 }
API-->>Client: 403 Forbidden
end
end
alt DELETE /api/projects/[projectId]/ai/sessions/[sessionId]
Client->>API: DELETE /ai/sessions/[id]
API->>Auth: requireAuth()
API->>Auth: requireProjectAccess(user.id, projectId, "ADMIN")
alt ADMIN
Auth-->>API: { project }
API->>AI: delete session
AI-->>API: deleted
API-->>Client: 200 OK
else MEMBER or VIEWER
Auth-->>API: { error: 403 }
API-->>Client: 403 Forbidden
end
end
alt POST /api/projects/[projectId]/ai/sessions/[sessionId]/messages
Client->>API: POST /messages (create)
API->>Auth: requireAuth()
API->>Auth: requireProjectAccess(user.id, projectId, "MEMBER")
alt MEMBER or ADMIN
Auth-->>API: { project }
API->>AI: create message
AI-->>API: message
API-->>Client: 200 OK
else VIEWER
Auth-->>API: { error: 403 }
API-->>Client: 403 Forbidden
end
end
dark-sorceror
requested changes
Jul 11, 2026
dark-sorceror
left a comment
dark-sorceror
left a comment
Collaborator
There was a problem hiding this comment.
Great work! Small nits in test files, also in frontend/ui/src/features/ai-assistant/components/session-history.tsx:48, a minor detail: add a else branch to the delete handler so that a MEMBER on the new 403 on session delete has the error surfaced and hide the delete affordance for non-ADMIN roles.
…ADMINs, drop tautological tests
Contributor
Author
|
requested changes have been addressed |
…r diff-cover gate
dark-sorceror
requested changes
Jul 13, 2026
dark-sorceror
left a comment
dark-sorceror
left a comment
Collaborator
There was a problem hiding this comment.
Previous reviews were addressed but the new fixes introduce some new issues.
- Use useWorkspace(workspaceId) to read current user role directly instead of fetching full member roster + useSession; derives isAdmin and isMember in AiAssistantPanel - Disable MessageInput for VIEWER role (non-MEMBER) - Default canDelete to false in SessionHistory (deny-by-default) - Add status field to AISession test fixture (was missing required field) - Propagate 403 into error bubbles for detector DELETE, Save, and New flows; hide New Detector button and Delete action for non-MEMBERs - Update tests: mock useWorkspace instead of workspace-members query, add VIEWER coverage for DetectorsPage and DetectorPanel
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 10 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Backend DELETE /detectors/[id] requires ADMIN (requireProjectAccess ADMIN). The round-2 fix incorrectly keyed the Delete button on isMember (MEMBER|ADMIN), meaning a MEMBER could see Delete and would silently get a 403 on click. Derive isAdmin separately from workspace.role and use it for the Delete action in the row's actions popover, keeping New Detector/Save gated at MEMBER.
Previously isMember defaulted to true when workspace was still undefined (loading), letting VIEWERs briefly see and click write controls before their role was known. The backend would 403 them anyway, but the UI gap contradicts the repo convention where unresolved role state is treated as read-only (same pattern isAdmin already follows). Changed all four isMember derivations to use optional-chaining so they evaluate to false until workspace resolves: workspace?.role === "MEMBER" || workspace?.role === "ADMIN" Updated detector-panel.test.tsx to set workspaceData = MEMBER on tests that click Save, and new/page.test.tsx workspace mock to return MEMBER (the role that should be able to create detectors).
Add error-state tests for the three detector mutation error banners added in round-2 review so the diff-cover >= 80% CI gate passes.
Contributor
Author
|
All changes from the latest review are addressed ! |
dark-sorceror
requested changes
Jul 17, 2026
dark-sorceror
left a comment
dark-sorceror
left a comment
Collaborator
There was a problem hiding this comment.
Small nit after that should be good
…nload 403) The prior run's Coverage Gate job hit a 403 downloading the coverage-frontend artifact seconds after it was uploaded (see PR comment) — not a real coverage regression. Empty commit to force a fresh run since re-running the failed job requires admin rights I don't have on this repo.
Contributor
Author
|
now that issue is resolved ! |
dark-sorceror
requested changes
Jul 18, 2026
| <Trash2 className="h-3.5 w-3.5" /> | ||
| Delete | ||
| </button> | ||
| {isAdmin && ( |
Collaborator
There was a problem hiding this comment.
The {isAdmin && (...)} gate on the Delete action has no role-boundary test. MEMBER appears nowhere in page.test.tsx, and no test opens the actions Popover
dark-sorceror flagged that MEMBER appeared nowhere in page.test.tsx and
no test actually opened the row actions Popover, so the {isAdmin && (...)}
gate on Delete had no role-boundary coverage.
Adds two tests that open the Popover via its trigger button and assert:
MEMBER sees Edit but not Delete; ADMIN sees both.
Contributor
Author
|
@dark-sorceror addressed in 911c53c — added two tests to
|
dark-sorceror's round-2 comment on ai/sessions/route.ts said "propagate the failure into an error bubble and disable the input for non-MEMBERs." Only the disable-input half was ever implemented -- ensureSession()'s catch block just did console.error(err) and returned null, so a 403 (or any other session-creation failure) silently no-op'd the send button with zero user-visible feedback. Adds a sendError state to useAiChat, set from the response body's error message (matching the pattern already used in session-history.tsx's delete handler and detector-panel.tsx's save handler), cleared on the next send attempt or a fresh session, and displayed in AiAssistantPanel next to the input. Purely additive: no existing behavior changed, 3 files touched.
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 4/5
- In
frontend/ui/src/features/ai-assistant/hooks/use-ai-chat.ts,handleCloseandhandleSelectSessiondon’t clearsendError, so a stale error state can persist when switching or reopening chats and mislead users into thinking the new session failed — clearsendErrorin those handlers (or on session/context change) to keep error banners accurate.
You’re at about 97% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/ui/src/features/ai-assistant/hooks/use-ai-chat.ts">
<violation number="1" location="frontend/ui/src/features/ai-assistant/hooks/use-ai-chat.ts:33">
P2: sendError is cleared in handleSend and handleNewSession but not in handleClose or handleSelectSession, so a stale error banner (e.g. a previous 403) can keep showing after the user closes the panel or switches to another session. Consider calling setSendError(null) in both handlers as well.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // Surfaces a failed session creation (e.g. a non-MEMBER's 403) instead of the | ||
| // previous silent console.error — the send button would otherwise no-op with | ||
| // no visible feedback. Cleared on the next send attempt or a fresh session. | ||
| const [sendError, setSendError] = useState<string | null>(null); |
Contributor
There was a problem hiding this comment.
P2: sendError is cleared in handleSend and handleNewSession but not in handleClose or handleSelectSession, so a stale error banner (e.g. a previous 403) can keep showing after the user closes the panel or switches to another session. Consider calling setSendError(null) in both handlers as well.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/ui/src/features/ai-assistant/hooks/use-ai-chat.ts, line 33:
<comment>sendError is cleared in handleSend and handleNewSession but not in handleClose or handleSelectSession, so a stale error banner (e.g. a previous 403) can keep showing after the user closes the panel or switches to another session. Consider calling setSendError(null) in both handlers as well.</comment>
<file context>
@@ -27,6 +27,10 @@ export function useAiChat({
+ // Surfaces a failed session creation (e.g. a non-MEMBER's 403) instead of the
+ // previous silent console.error — the send button would otherwise no-op with
+ // no visible feedback. Cleared on the next send attempt or a fresh session.
+ const [sendError, setSendError] = useState<string | null>(null);
// Reset session + messages when the user navigates to a different project so
</file context>
Full re-verification of every review round on this PR turned up two untested code paths the Coverage Gate flagged on the sendError commit: - detectors/page.tsx: the empty-project state has its own isMember-gated "New Detector" button (distinct from the header's), never rendered by any test since the mock data always had detectors. The Delete row action's onClick handler was verified visible for ADMIN but never actually clicked, so its body never executed. - use-ai-chat.ts: had zero test coverage of any kind before the sendError fix -- no existing test exercises the real hook, only a mocked context. Adds: - Two detectors/page.test.tsx cases covering the empty-state button for MEMBER+ and its absence for VIEWER, plus a Delete-click test that captures DeleteDetectorDialog's props to confirm the right detector was targeted. - A new use-ai-chat.test.tsx covering ensureSession's error path (body message, generic fallback, clearing on next send / new session). Local diff-cover against upstream/main: 100% (93/93 lines), matching what the Coverage Gate computes in CI. 901/903 full suite passes (2 pre-existing, unrelated flakes: locale-dependent formatting test, Slack OAuth timeout).
Contributor
Author
|
@dark-sorceror all the changes are incorporated . Could you please review this PR ? |
…-github-token # Conflicts: # frontend/ui/src/app/projects/[projectId]/detectors/page.test.tsx # frontend/ui/src/app/projects/[projectId]/detectors/page.tsx
…-github-token # Conflicts: # frontend/ui/src/features/ai-assistant/components/ai-assistant-panel.test.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the role-gate gap reported in #1504.
Root fix:
GET /api/github/tokensession branch now callsrequireWorkspaceMembership(..., "ADMIN")— previously it passed nominRole, so any workspace VIEWER could obtain a live GitHub App installation token scoped to the customer's connected repos. The internal-secret branch is unaffected.Same-class sweep across sibling routes:
github/tokenprojects/[id]/detectorsprojects/[id]/detectors/[id]projects/[id]/detectors/[id]projects/[id]/ai/sessionsprojects/[id]/ai/sessions/[id]projects/[id]/ai/sessions/[id]/messagesRead-only GET routes are intentionally unchanged.
Test plan
minRolestring passed torequireProjectAccess/requireWorkspaceMembershipGET /api/github/tokenin a staging workspace with GitHub connectedCloses #1504
Summary by cubic
Locks down write routes and prevents VIEWERs from minting GitHub App tokens. The UI now respects roles, hides write actions until the role is known, and surfaces clear error messages. Closes #1504.
GET /api/github/token(session) now requires ADMIN; internal-secret path unchanged; VIEWERs get 403 and no token is minted.useWorkspace; disable MessageInput for VIEWERs and while role is unresolved; passcanDeleteonly for ADMIN (default false); surface 403/network errors in SessionHistory; show session-creation/send errors near the input.canDelete/delete, send-error handling inuse-ai-chat, row actions Popover (MEMBER sees Edit only; ADMIN sees Delete), and empty-state New button gating.Written for commit b3617ba. Summary will update on new commits.